Skip to content

[DRAFT] autoimport #1553

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

[DRAFT] autoimport #1553

wants to merge 4 commits into from

Conversation

iisaduan
Copy link
Member

@iisaduan iisaduan commented Aug 11, 2025

in-progress port of autoimport completions

also going to do some code cleanup/reorganizing, I wasn't sure how much code each thing would have when i made the files/chose which file to put things in in the first place

@alexwork1611
Copy link

I'm gonna closely follow this one...

@iisaduan
Copy link
Member Author

iisaduan commented Aug 11, 2025

bug squashing atm :). Hopefully it'll be ready soon

@@ -95,8 +98,8 @@ type completionDataData struct {
isJsxIdentifierExpected bool
isRightOfOpenTag bool
isRightOfDotOrQuestionDot bool
importStatementCompletion any // !!!
hasUnresolvedAutoImports bool // !!!
importStatementCompletion *importStatementCompletionInfo // !!!
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can drop the // !!! here now I think

// Unless this option is `false`, member completion lists triggered with `.` will include entries
// on potentially-null and potentially-undefined values, with insertion text to replace
// preceding `.` tokens with `?.`.
IncludeAutomaticOptionalChainCompletions *bool

// If enabled, the completion list will include completions with invalid identifier names.
// For those entries, The `insertText` and `replacementSpan` properties will be set to change from `.x` property access to `["x"]`.
IncludeCompletionsWithInsertText *bool
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need this anymore, every LSP client should support this.

// Enables auto-import-style completions on partially-typed import statements. E.g., allows
// `import write|` to be completed to `import { writeFile } from "fs"`.
IncludeCompletionsForImportStatements *bool

// Allows completions to be formatted with snippet text, indicated by `CompletionItem["isSnippet"]`.
IncludeCompletionsWithSnippetText *bool
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also don't need this as a user preference. Instead, we need to check completion client capabilities for snippetSupport.

moduleSymbol,
file,
position,
"", // entryId.data.exportMapKey
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are these comments for? Is it the corresponding Strada code?

}

// If some file is using ES6 modules, assume that it's OK to add more.
return true || // !!! symlink program.getSymlinkCache?.().hasAnySymlinks() ||
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be false while the symlink condition isn't implemented yet?

@@ -731,7 +731,7 @@ func (f *FourslashTest) verifyCompletionsAreExactly(t *testing.T, prefix string,
var completionIgnoreOpts = cmp.FilterPath(
func(p cmp.Path) bool {
switch p.Last().String() {
case ".Kind", ".SortText", ".Data":
Copy link
Member

@gabritto gabritto Aug 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have completion tests that test filterText, so we shouldn't always ignore it. If you need to ignore it for some tests, I'd suggest adding an explicit ignore option like we do for other properties (see ExpectedCompletionEditRange). I think ideally we'd do what we did in Strada where we checked if the property was defined, but since that's not a thing in Go it's trickier to allow for testing presence/absence of filterText when we want to but ignore it the rest of the time. If it's just for a few tests, we can probably manually fill in filterText, otherwise we can set up an explicit ignore. I've been meaning to do that for a while now, let me know if you want me to do that and what tests are failing without this change it.

@@ -2428,6 +2428,10 @@ func GetLineStarts(sourceFile ast.SourceFileLike) []core.TextPos {
return sourceFile.LineMap()
}

func GetLineStartPositionForPosition(pos int, lineMap []core.TextPos) int {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if it does what you need, but we have a function with the same name in internal/format/util.go.

return true
}

var prevChar *rune
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect you might be able to get away with using 0 here, given an identifer can't contain NUL.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants